Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polyfill AbortSignal.any in PDF.js legacy builds #19218

Closed

Conversation

Snuffleupagus
Copy link
Collaborator

With recent changes, in particular PR #19216, we're now relying on AbortSignal.any in a way that cannot just be skipped with pre-processor statements as done previously.
This is problematical since AbortSignal.any is fairly new functionality, see https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/any_static#browser_compatibility, that's not available in all browsers that we support.

Instead it seems that we need to add a polyfill, however I unfortunately wasn't able to find one on NPM. In fact the only one I could find was https://gist.github.com/CNSeniorious000/9fc1a72e45358dd7c9e2f16e5d26df5c, which doesn't contain any licensing information.

Note: Given the use of pre-processor statements, this polfill will not be included in the Firefox PDF Viewer.

With recent changes, in particular PR 19216, we're now relying on `AbortSignal.any` in a way that cannot just be skipped with pre-processor statements as done previously.
This is problematical since `AbortSignal.any` is fairly new functionality, see https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/any_static#browser_compatibility, that's not available in all browsers that we support.

Instead it seems that we need to add a polyfill, however I unfortunately wasn't able to find one on NPM. In fact the only one I could find was https://gist.github.com/CNSeniorious000/9fc1a72e45358dd7c9e2f16e5d26df5c, which doesn't contain any licensing information.

*Note:* Given the use of pre-processor statements, this polfill will *not* be included in the Firefox PDF Viewer.
@Snuffleupagus Snuffleupagus force-pushed the AbortSignal-any-polyfill branch from e206def to dcca4d0 Compare December 12, 2024 11:02
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/bc4b3e916b58494/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/1e1346ffc48d657/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/1e1346ffc48d657/output.txt

Total script time: 28.42 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/bc4b3e916b58494/output.txt

Total script time: 52.07 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

@Snuffleupagus Snuffleupagus added the release-blocker Blocker for the upcoming release label Dec 13, 2024
@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Dec 15, 2024

Note that the only "alternative" to this PR would be to raise the minimum supported browser versions very significantly, see https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/any_static#browser_compatibility, which I cannot imagine would go over particularly well (since users already somewhat regularly complain about the requirements).

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me. @calixteman Could you comment on the licensing bit here before we land this?

@sylvestre
Copy link
Contributor

sorry, if there isn't any licenses associated to this code, it can't be merged.
You should either find a different implementation or ask the developer to relicense it.

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Dec 15, 2024

sorry, if there isn't any licenses associated to this code, it can't be merged.

Just in case it's not clear, please note that the code will not be included in the Firefox PDF Viewer.

You should either find a different implementation

As mentioned in #19218 (comment), there doesn't appear to be one.

or ask the developer to relicense it.

Sorry, but that's then more work than I'm willing to put into an issue that doesn't affect me.

Hence it seems that we need to move forward with the alternative outlined in #19218 (comment), which is bumping the supported browsers significantly (and probably make that an api-major change).

@sylvestre
Copy link
Contributor

Just in case it's not clear, please note that the code will not be included in the Firefox PDF Viewer.

Even with that, we can't take the risk. Sorry

@Snuffleupagus Snuffleupagus deleted the AbortSignal-any-polyfill branch December 15, 2024 15:27
@Snuffleupagus Snuffleupagus removed the release-blocker Blocker for the upcoming release label Dec 15, 2024
@timvandermeij timvandermeij removed the request for review from calixteman December 15, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants